Remarshal manifest to decrease false-positive #3370
Conversation
|
I'll add test |
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
Code coverage for golang is
|
| } | ||
|
|
||
| old.u = remarshal(old.u) | ||
| new.u = remarshal(new.u) |
There was a problem hiding this comment.
Why do we need to remarshal this one?
There was a problem hiding this comment.
I think this is not needed(if I delete new.u ..., it works fine) but argoproj does like this. So I wrote like this.
There was a problem hiding this comment.
Got it. I guess re-marshal the new one to ensure that both of them are in the same struct model to be more comparable.
But let's go without that to see what happens. 😄
There was a problem hiding this comment.
Got it. I guess re-marshal the new one to ensure that both of them are in the same struct model to be more comparable.
But let's go without that to see what happens. 😄
| func remarshal(obj *unstructured.Unstructured) *unstructured.Unstructured { | ||
| data, err := json.Marshal(obj) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Returning an error to handle instead of panic.
| item, err := scheme.Scheme.New(obj.GroupVersionKind()) | ||
| if err != nil { | ||
| // This is common. the scheme is not registered | ||
| log.Printf("Could not create new object of type %s: %v", gvk, err) |
There was a problem hiding this comment.
Don't use this log package.
Return the error and use our logger to log.
There was a problem hiding this comment.
This error is common. So can I ignore this error and simply return input object?
There was a problem hiding this comment.
@Hosshii I mean we can just return the error and add the log here
There was a problem hiding this comment.
Does this mean that when there is an error, it returns both the error and the object?
I mean that if error is occurred, return original object and error, then caller side print the error and continue process.
There was a problem hiding this comment.
When there is an error, return only the error.
And at the caller side, if it gets an error, it logs the error message and uses the original object to compare.
func Diff(old, new Manifest, opts ...diff.Option) (*diff.Result, error) {
normalizedOld, err := remarshal(old.u)
if err != nil {
log()
} else {
old.U = normalizedOld
}
}
func remarshal(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {There was a problem hiding this comment.
I got your point.
Thank you!
There was a problem hiding this comment.
I think Diff function does not have logger, so we have to pass it to Diff and some other function. Is it ok?
|
Code coverage for golang is
|
|
You may need to run |
|
I forgot to rename the file. |
|
Code coverage for golang is
|
| for _, tc := range testcases { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Thank you! By adding this, t.Run also runs concurrently. But this test is fast and so all t.parallel may not needed
There was a problem hiding this comment.
tbh, I think if we overdo on using routine, prepare for those routines may cause slower execution. So maybe, we can use parallel on test function level or test cases level only, wdyt? 🤔
There was a problem hiding this comment.
Agree about that. As the first step, parallel only at the high level (test function level) could be enough.
There was a problem hiding this comment.
We can use -parallel option in order to control that.
That's why, we needn't to think about it every time.
There was a problem hiding this comment.
Thank you!
I'll just use t.parallel at the high level for now.
|
Thank you! I fixed, PTAL. By the way, this error is common and logging this may noisy. How do you think? |
|
/rebase |
|
@nghialv: Unfortunately, you have to rebase this pull request manually. Because GitHub does not think that this pull request is rebaseable automatically. |
|
Code coverage for golang is
|
|
I fixed. PTAL |
|
/lgtm |
|
Thank you! |
Co-authored-by: Le Van Nghia <[email protected]>
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
Code coverage for golang is
|
Co-authored-by: Le Van Nghia <[email protected]>
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
The following files are not gofmt-ed. By commenting pkg/app/piped/cloudprovider/kubernetes/diff.go--- pkg/app/piped/cloudprovider/kubernetes/diff.go.orig
+++ pkg/app/piped/cloudprovider/kubernetes/diff.go
@@ -62,7 +62,7 @@
normalized, err := remarshal(old.u)
if err != nil {
logger.Info("Unable to remarshal Kubernetes manifest, the raw data will be used to calculate the diff", zap.Error(err))
- return diff.DiffUnstructureds(*old.u, *new.u, opts...)
+ return diff.DiffUnstructureds(*old.u, *new.u, opts...)
}
return diff.DiffUnstructureds(*normalized, *new.u, opts...)
|
|
/golinter fmt |
|
Nice improvement. Thank you. /approve |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
|
Code coverage for golang is
|
What this PR does / why we need it:
To reduce wrong diff result, this PR introduce
remarshalfunction that marshal the object to struct that defined by k8s, and then return it to our object. This function is copied from argoproj/gitops-engin and modified.Which issue(s) this PR fixes:
Fixes #3334
Does this PR introduce a user-facing change?: